Langtag download issue 987 #1232
Conversation
Implement sillsdev#987. +semver:minor
SIL.WritingSystems/Sldr.cs
Outdated
| { | ||
| etag = File.ReadAllText(etagPath); | ||
| currentEtag = webResponse.Headers.Get("Etag"); | ||
| if (etag == "") |
There was a problem hiding this comment.
No need to check for empty etag after we got the Etag header. You can unconditionally write the new value (but maybe do it in the webResponse.StatusCode != HttpStatusCode.NotModified block).
SIL.WritingSystems/Sldr.cs
Outdated
| else | ||
| sinceTime = fileTime; | ||
| } | ||
| sinceTime += TimeSpan.FromSeconds(1); |
There was a problem hiding this comment.
Since we don't send sinceTime to the server, we no longer need that variable here - we only need it above in line 419 so that we can compare it to fileTime.
SIL.WritingSystems/Sldr.cs
Outdated
| { | ||
| LoadLanguageTagsIfNecessary(); | ||
| if (downloadLangTags) LoadLanguageTags(); |
There was a problem hiding this comment.
We should download the tags before we call LoadLanguageTagsIfNecessary
SIL.WritingSystems/Sldr.cs
Outdated
| webRequest.Timeout = 10000; | ||
| webRequest.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate; | ||
| using var webResponse = (HttpWebResponse) webRequest.GetResponse(); | ||
| public static void LoadLanguageTags() |
There was a problem hiding this comment.
We now have two methods with almost identical names (LoadLanguageTags and LoadLanguageTagsIfNecessary) which do different things which is confusing. Maybe rename this method to DownloadLanguageTags?
elisunger
force-pushed
the
langtagDownload-issue-987
branch
from
97fe7fd to
4385e10
Compare
There was a problem hiding this comment.
We're getting closer. There are a few things that still need to be changed.
I also added some unit tests that you can use to debug and verify the correct behavior. You'll have to run git pull [email protected]:elisunger/libpalaso.git --rebase langtagDownload-issue-987 to get them.
SIL.WritingSystems/Sldr.cs
Outdated
| if (File.Exists(etagPath)) | ||
| { | ||
| etag = File.ReadAllText(etagPath); | ||
| webRequest.Headers.Set(etag, "If-None-Match"); |
There was a problem hiding this comment.
This is backwards. etag is the value, "If-None-Match" the key.
| webRequest.Headers.Set(etag, "If-None-Match"); | |
| webRequest.Headers.Set("If-None-Match", etag); |
SIL.WritingSystems/Sldr.cs
Outdated
| webRequest.Timeout = 10000; | ||
| webRequest.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate; | ||
| using var webResponse = (HttpWebResponse)webRequest.GetResponse(); | ||
| if (File.Exists(etagPath)) |
There was a problem hiding this comment.
You'll also have to check for the existence of langtags.json file - it's possible that langtags.json.etag exists but langtags.json doesn't (e.g. it gets renamed if it contains corrupt content).
SIL.WritingSystems/Sldr.cs
Outdated
| { | ||
| //File.WriteAllText(etagPath, currentEtag); | ||
| //webRequest.Headers.Set(etag, "If-None-Match"); | ||
| if (webResponse.StatusCode != HttpStatusCode.NotModified) |
There was a problem hiding this comment.
The status code should be checked directly after calling webRequest.GetResponse(). If we get a status code other than OK we can return right away: either it's NotModified in which case we don't have to do anything, or we get an error in which case the data we got is not valid, so we can't do anything with it either.
In all cases I saw in my testing we got a WebException instead of a non-OK status code, but since the code comment says we might get a NotModified status code it safer to check it.
SIL.WritingSystems/Sldr.cs
Outdated
| File.WriteAllText(etagPath, currentEtag); | ||
| using Stream output = File.OpenWrite(cachedAllTagsPath); | ||
| using var input = webResponse.GetResponseStream(); | ||
| input.CopyTo(output); |
There was a problem hiding this comment.
It's possible that webResponse.GetResponseStream() returns null, so you should check for null here. This can be easiest done as:
| input.CopyTo(output); | |
| input?.CopyTo(output); |
SIL.WritingSystems/Sldr.cs
Outdated
| if (_offlineMode) | ||
| throw new WebException("Test mode: SLDR offline so accessing cache", WebExceptionStatus.ConnectFailure); |
There was a problem hiding this comment.
These two lines can go directly below CreateSldrCacheDirectory().
| webRequest.Timeout = 10000; | ||
| webRequest.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate; | ||
| using var webResponse = (HttpWebResponse)webRequest.GetResponse(); | ||
| if (webResponse.StatusCode != "OK") |
There was a problem hiding this comment.
StatusCode returns an enum, so you can't compare to string:
| if (webResponse.StatusCode != "OK") | |
| if (webResponse.StatusCode != HttpStatusCode.OK) |
| var langtagsUrl = | ||
| $"{SldrRepository}index.html?query=langtags&ext=json{StagingParameter}"; | ||
| var webRequest = (HttpWebRequest)WebRequest.Create(Uri.EscapeUriString(langtagsUrl)); | ||
| webRequest.Headers.Set("If-None-Match", etag); |
There was a problem hiding this comment.
The compiler doesn't like this line and fails with Error CS0165 : Use of unassigned local variable 'etag'. You should read etagPath before this line (you can copy lines 525 and 527).
This change is